feat(core): Deprecate Span.setHttpStatus in favor of setHttpStatus
#10268
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
size-limit report 📦
|
mydea
approved these changes
Jan 19, 2024
Lms24
force-pushed
the
lms/feat-core-deprecate-span-setHttpStatus
branch
from
7a593a7 to
b681a47
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR deprecates the last remaining otel-incompatible API on the
Spanclass and interface -Span.setHttpStatus.We replace this functionality with a
setHttpStatuswrapper because in addition to setting the data/tag value (more on that below), this function also derives the span status from the status code. Added tests to make sure this is covered properly (I think it wasn't covered well before and we need to remove the old tests anyway).I checked with the ingest team and theoretically, from a Relay PoV, we could stop sending the
http.status_codetag right now. Our min supported Relay version already primarily expectes thehttp.response.status_codedata property and only falls back to the tag. However: After giving this more thought, I opted to leave the payload unchanged for now because users can theoretically depend on the presence of the tag or data inbeforeSendTransaction. So to avoid breaking user-added behaviour, I'd leave things as they are in v7 and make the switch to setting the status code as an attribute in v8. This is easy anyway.ref #10184